Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(analysis): Graphite query - remove whitespaces #2752

Merged
merged 2 commits into from
May 9, 2023

Conversation

mdsjip
Copy link
Contributor

@mdsjip mdsjip commented Apr 30, 2023

If metric query contains surrounding whitespace, then it's not trimmed and Graphite host returns 400 Bad Request

Simplest example of analysis template:

apiVersion: argoproj.io/v1alpha1
kind: ClusterAnalysisTemplate
metadata:
  name: error-rate
  namespace: argo-rollouts
spec:
  metrics:
    - name: error-rate
      failureLimit: 1
      # Note that the Argo Rollouts Graphite metrics provider returns result as an array of float64s with 6 decimal places.
      successCondition: 'len(result) > 0 and result[len(result)-1] < asFloat(10)'
      provider:
        graphite:
          address: https://some-graphite-host.com
          query: |
            from=-2&target=sumSeries(app.http.*.*.count)

When this is parsed from YAML, this query contains a newline in the end, so when it's "trimmed", it still contains a trailing space, so when put into the query, it becomes:

format=json&from=-2min&target=sumSeries%28app.http.%2A.%2A.count%29+

This leads to 400 Bad Request response from Graphite, example cURL command:

$ curl 'https://some-graphite-host.com/render?format=json&from=-2min&target=sumSeries%28app.http.%2A.%2A.count%29+'
Bad Request: Bad Request

Target              : sumSeries(app.http.*.*.count) 
Parsed so far       : sumSeries(app.http.*.*.count)
Could not parse     :  

Proposed solution: replace all whitespace in the query with an empty string.

Checklist:

  • Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this is a chore.
  • The title of the PR is (a) conventional with a list of types and scopes found here, (b) states what changed, and (c) suffixes the related issues number. E.g. "fix(controller): Updates such and such. Fixes #1234".
  • I've signed my commits with DCO
  • I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
  • My builds are green. Try syncing with master if they are not.
  • My organization is added to USERS.md.

@github-actions
Copy link
Contributor

github-actions bot commented Apr 30, 2023

Go Published Test Results

1 952 tests   1 952 ✔️  2m 36s ⏱️
   118 suites         0 💤
       1 files           0

Results for commit 628842f.

♻️ This comment has been updated with latest results.

@codecov
Copy link

codecov bot commented Apr 30, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.02 ⚠️

Comparison is base (c74c464) 81.64% compared to head (580a38c) 81.62%.

❗ Current head 580a38c differs from pull request most recent head 628842f. Consider uploading reports for the commit 628842f to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2752      +/-   ##
==========================================
- Coverage   81.64%   81.62%   -0.02%     
==========================================
  Files         133      133              
  Lines       20172    20157      -15     
==========================================
- Hits        16469    16454      -15     
  Misses       2849     2849              
  Partials      854      854              
Impacted Files Coverage Δ
metricproviders/graphite/api.go 78.04% <100.00%> (-0.78%) ⬇️

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@github-actions
Copy link
Contributor

github-actions bot commented Apr 30, 2023

E2E Tests Published Test Results

    4 files      4 suites   3h 29m 43s ⏱️
  96 tests   84 ✔️   5 💤   7
400 runs  365 ✔️ 20 💤 15

For more details on these failures, see this check.

Results for commit 628842f.

♻️ This comment has been updated with latest results.

@mdsjip mdsjip force-pushed the graphite-trim-fix branch 2 times, most recently from e97d01c to ef90c16 Compare May 6, 2023 12:38
@sonarcloud
Copy link

sonarcloud bot commented May 6, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
5.5% 5.5% Duplication

@mdsjip mdsjip requested a review from zachaller May 6, 2023 12:47
@zachaller zachaller merged commit 0b412b8 into argoproj:master May 9, 2023
@mdsjip mdsjip deleted the graphite-trim-fix branch May 27, 2023 16:46
@mdsjip
Copy link
Contributor Author

mdsjip commented May 27, 2023

@zachaller will these changes be included in the next release soon? 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants